-
Notifications
You must be signed in to change notification settings - Fork 195
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Relay IDFA information through consent plugin #906
Conversation
|
||
let lastDeviceAttrs = analytics.context.get()?.device; | ||
analytics.context.onChange((c) => { | ||
const newAttrs = c?.device; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any reason why not check the specific values inside context.device
? (e.g. AdTrackingEnabled, AdvertisingId)
device
context contains a lot more values than ATT so those might change and trigger a notifyConsentChange without necessarily being an ad tracking related thing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, that was how my original implementation worked but I feared that in case more metadata becomes subject of IDFA, we might forget to also update this plugin as well.
Most fields in device
are static and very unlikely to updated during the runtime of the app.
wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better to constraint it to the specific fields.
It's more likely that we extend context
with something not IDFA related and might be harder to spot that this has a side effect if it's not directly named. We also have to take into account that plugins can modify the context same as IDFA/AdvertisingID so we cannot control for what other devs are doing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
🎉 This PR is included in version 1.1.1 🎉 The release is available on npm package (@latest dist-tag) Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 2.19.0 🎉 The release is available on npm package (@latest dist-tag) Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 1.3.1 🎉 The release is available on npm package (@latest dist-tag) Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 1.1.1 🎉 The release is available on npm package (@latest dist-tag) Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 0.6.1 🎉 The release is available on npm package (@latest dist-tag) Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 1.1.1 🎉 The release is available on npm package (@latest dist-tag) Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 0.4.1 🎉 The release is available on npm package (@latest dist-tag) Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 0.7.1 🎉 The release is available on npm package (@latest dist-tag) Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 1.2.0 🎉 The release is available on npm package (@latest dist-tag) Your semantic-release bot 📦🚀 |
No description provided.